Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Add quick-start guide. #36

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kirkrodrigues
Copy link
Member

@kirkrodrigues kirkrodrigues commented Dec 17, 2024

Description

This PR adds a quick-start guide for writing a basic task and running it on Spider. In future PRs, we will explain how to write complex tasks (e.g., tasks joined together into task graph) as well as how to use Spider's data and key-value stores.

NOTE: This should not be merged until the scheduler and worker can be run as standalone processes.

Validation performed

  • Validated the output renders correctly on GitHub.
  • Validated the links are correct.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Introduced a comprehensive quick-start guide for users to effectively utilize the Spider distributed system.
  • Documentation
    • Added a new documentation file detailing steps for writing tasks, building shared libraries, and setting up a Spider cluster, including prerequisites and example tasks.

Copy link

coderabbitai bot commented Dec 17, 2024

Walkthrough

The pull request introduces a new documentation file docs/quick-start.md that provides a comprehensive guide for users to quickly start using the Spider distributed system. The document covers the entire process of creating and executing tasks, from writing task functions to setting up a Spider cluster. It includes detailed instructions on system requirements, task implementation, shared library creation, client application development, and cluster configuration.

Changes

File Change Summary
docs/quick-start.md New documentation file added with a detailed quick-start guide for the Spider distributed system.

Sequence Diagram

sequenceDiagram
    participant User as User
    participant Task as Task Function
    participant Client as Spider Client
    participant Cluster as Spider Cluster
    
    User->>Task: Implement task function
    User->>Task: Build shared library
    User->>Client: Write client application
    Client->>Cluster: Connect to cluster
    Client->>Cluster: Submit task
    Cluster->>Task: Execute task
    Task-->>Cluster: Return result
    Cluster-->>Client: Provide result
Loading

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (5)
docs/quick-start.md (5)

21-25: Consider enhancing the requirements section

A few suggestions to improve clarity:

  • Add minimum version requirements for CMake
  • Consider adding approximate disk space requirements for Docker images
  • Format the Docker link reference to be consistent with the text (i.e., use [Docker] in line 23)

71-71: Replace hard tab with spaces

The line contains a hard tab character. For consistency, use spaces for indentation.

-		return x + y;
+    return x + y;
🧰 Tools
🪛 Markdownlint (0.37.0)

71-71: Column: 1
Hard tabs

(MD010, no-hard-tabs)


29-34: Consider expanding on TaskContext usage

While the note mentions that TaskContext will be explained later, it would be helpful to provide a brief description of its purpose here, as it's a required parameter for all tasks.


254-266: Add execution verification steps

Consider adding:

  • Expected output for successful execution
  • Common error messages and their resolution
  • How to verify the task was executed on the cluster

267-271: Enhance the next steps section

Consider adding:

  • Links to advanced topics (even if the pages don't exist yet)
  • Brief overview of what fault tolerance means in Spider
  • Examples of more complex tasks users might want to try
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38297fc and 3ef6f42.

📒 Files selected for processing (1)
  • docs/quick-start.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/quick-start.md

[uncategorized] ~165-~165: Possible missing comma found.
Context: ...ent can be compiled like any normal C++ application except that we need to link it to the S...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint (0.37.0)
docs/quick-start.md

71-71: Column: 1
Hard tabs

(MD010, no-hard-tabs)


46-46: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


62-62: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


101-101: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

Comment on lines 83 to 89
# Building the task into a shared library

In order for Spider to run a task, the task needs to be compiled into a shared library that Spider
can load.

TODO: Instructions and an example showing how to compile tasks into a shared library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Add missing build instructions

The TODO marker indicates missing instructions for compiling tasks into a shared library. This is a blocking issue as users cannot proceed without these instructions. Consider adding:

  • CMake configuration examples
  • Compiler flags for shared library creation
  • Output location expectations

Would you like me to help draft the missing build instructions section?

switch (job_status) {
case JobStatus::Succeeded: {
auto result = job_status.get_result();
if (result == job_status.get_result()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the result comparison logic

The condition is comparing the result with itself, which will always be true:

if (result == job_status.get_result())

Should be:

-if (result == job_status.get_result())
+if (result == 5)  // Expected sum of 2 + 3

Comment on lines 163 to 169
# Building the client

The client can be compiled like any normal C++ application except that we need to link it to the
Spider client library.

TODO: Instructions and an example for how to compile the client application.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Add missing client build instructions

The TODO marker indicates missing instructions for compiling the client application. This is a blocking issue as users cannot proceed without these instructions. Consider adding:

  • CMake configuration examples
  • Required Spider library linking flags
  • Build system requirements

Would you like me to help draft the missing client build instructions section?

🧰 Tools
🪛 LanguageTool

[uncategorized] ~165-~165: Possible missing comma found.
Context: ...ent can be compiled like any normal C++ application except that we need to link it to the S...

(AI_HYDRA_LEO_MISSING_COMMA)

Comment on lines +189 to +193
--env MARIADB_USER=spider \
--env MARIADB_PASSWORD=password \
--env MARIADB_DATABASE=spider-storage \
--env MARIADB_ALLOW_EMPTY_ROOT_PASSWORD=true \
--publish 3306:3306 mariadb:latest
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add security warning for credentials

The example uses hardcoded credentials in plain text. Consider adding a security note about:

  • Using environment variables for credentials
  • Implementing proper authentication in production
  • Using secure password practices

To start the scheduler, run:

```shell
build/src/spider/spider_schdeuler \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo in scheduler command

There's a typo in the executable name.

-build/src/spider/spider_schdeuler \
+build/src/spider/spider_scheduler \
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
build/src/spider/spider_schdeuler \
build/src/spider/spider_scheduler \

Comment on lines 139 to 141
std::cerr << "`sum` returned unexpected result. Expected: 5. Actual: " << result
<< '\n'.
return 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix syntax error in error message

There's an invalid period after the string concatenation.

-                            << '\n'.
+                            << '\n';
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
std::cerr << "`sum` returned unexpected result. Expected: 5. Actual: " << result
<< '\n'.
return 1;
std::cerr << "`sum` returned unexpected result. Expected: 5. Actual: " << result
<< '\n';
return 1;

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (4)
docs/quick-start.md (4)

157-157: ⚠️ Potential issue

Fix the result comparison logic

The condition is comparing the result with itself, which will always be true.

-if (result == job_status.get_result())
+if (result == 5)  // Expected sum of 2 + 3

160-162: ⚠️ Potential issue

Fix syntax error in error message

There's an invalid period after the string concatenation.

-                            << '\n'.
+                            << '\n';

231-235: ⚠️ Potential issue

Add security warning for credentials

The example uses hardcoded credentials in plain text. Consider adding a security note about:

  • Using environment variables for credentials
  • Implementing proper authentication in production
  • Using secure password practices

257-257: ⚠️ Potential issue

Fix typo in scheduler command

There's a typo in the executable name.

-build/src/spider/spider_schdeuler \
+build/src/spider/spider_scheduler \
🧹 Nitpick comments (4)
docs/quick-start.md (4)

29-29: Fix grammatical error

Add the missing article "a":

-In Spider, a task is C++ function that satisfies the following conditions:
+In Spider, a task is a C++ function that satisfies the following conditions:
🧰 Tools
🪛 LanguageTool

[uncategorized] ~29-~29: Possible missing article found.
Context: ... # Writing a task In Spider, a task is C++ function that satisfies the following...

(AI_HYDRA_LEO_MISSING_A)


71-71: Replace hard tabs with spaces

Replace the hard tab with spaces to maintain consistent formatting throughout the document.

🧰 Tools
🪛 Markdownlint (0.37.0)

71-71: Column: 1
Hard tabs

(MD010, no-hard-tabs)


46-122: Use proper heading structure for file names

Instead of using emphasis (_) for file names, consider using a proper heading structure:

-_tasks.hpp_
+### tasks.hpp

-_tasks.cpp_
+### tasks.cpp

-_client.cpp_
+### client.cpp
🧰 Tools
🪛 Markdownlint (0.37.0)

71-71: Column: 1
Hard tabs

(MD010, no-hard-tabs)


46-46: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


62-62: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


122-122: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


186-186: Add missing commas for better readability

Add commas in the following sentences:

-The client can be compiled like any normal C++ application except that we need to link it to the Spider client library.
+The client can be compiled like any normal C++ application, except that we need to link it to the Spider client library.

-In future guides, we'll explain how to write more complex tasks as well as how to leverage Spider's support for fault tolerance.
+In future guides, we'll explain how to write more complex tasks, as well as how to leverage Spider's support for fault tolerance.

Also applies to: 311-311

🧰 Tools
🪛 LanguageTool

[uncategorized] ~186-~186: Possible missing comma found.
Context: ...ent can be compiled like any normal C++ application except that we need to link it to the S...

(AI_HYDRA_LEO_MISSING_COMMA)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ef6f42 and afd3454.

📒 Files selected for processing (1)
  • docs/quick-start.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/quick-start.md

[uncategorized] ~29-~29: Possible missing article found.
Context: ... # Writing a task In Spider, a task is C++ function that satisfies the following...

(AI_HYDRA_LEO_MISSING_A)


[uncategorized] ~186-~186: Possible missing comma found.
Context: ...ent can be compiled like any normal C++ application except that we need to link it to the S...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~311-~311: Possible missing comma found.
Context: ...we'll explain how to write more complex tasks as well as how to leverage Spider's sup...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint (0.37.0)
docs/quick-start.md

71-71: Column: 1
Hard tabs

(MD010, no-hard-tabs)


46-46: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


62-62: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


122-122: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
docs/quick-start.md (1)

269-269: Fix typo in documentation

There's a typographical error in the word "ihn".

-  `storage_url` argument ihn the command.
+  `storage_url` argument in the command.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between afd3454 and 2cb34a8.

📒 Files selected for processing (1)
  • docs/quick-start.md (1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/quick-start.md

46-46: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


62-62: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


122-122: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

🔇 Additional comments (2)
docs/quick-start.md (2)

160-160: Fix the result comparison logic

The condition is comparing job_status.get_result() with itself instead of the expected result.

Apply this diff to fix the comparison:

-if (expected == job_status.get_result()) {
+if (result == expected) {

234-238: Add security warning about credential handling

The examples use hardcoded credentials in plain text. This could lead to security issues if copied directly to production environments.

Consider adding a security note about:

  • Using environment variables for credentials
  • Implementing proper authentication in production
  • Using secure password practices

Also applies to: 262-262, 286-286, 304-304

Comment on lines 88 to 89
Suppose that spider is added in subdirectory `spider`, we can set up the following `CMakeLists.txt`
to build the task library:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add instructions for obtaining the Spider library

The guide assumes Spider is available in a subdirectory but doesn't explain how to obtain it. Consider adding instructions for:

  • Cloning the Spider repository
  • Setting up the library as a submodule
  • Alternative installation methods

Would you like me to help draft these additional setup instructions?

Also applies to: 192-193

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
docs/quick-start.md (5)

21-23: Consider adding version requirements

While the requirements section lists the necessary tools, it would be helpful to specify minimum version requirements for CMake and Docker.

Would you like me to help draft the version requirements section?


31-33: Enhance TaskContext parameter documentation

While there's a note about explaining TaskContext later, it would be helpful to provide a brief description of its purpose here, as it's a required parameter for all tasks.

Would you like me to help draft a brief TaskContext description?


169-170: Enhance error message clarity

Consider providing more descriptive error messages that include:

  • The specific operation that failed
  • Any relevant context (e.g., task name, parameters)
-            std::cerr << "Job failed in function " << error_and_fn_name.second << " - "
-                   << error_and_fn_name.first << '\n';
+            std::cerr << "Task execution failed: " << error_and_fn_name.first 
+                   << "\nFunction: " << error_and_fn_name.second
+                   << "\nParameters: x=" << x << ", y=" << y << '\n';

200-205: Specify the client executable location

Add a note about where to find the compiled client executable in the build directory.


293-297: Add example output

Consider adding example output showing:

  • Successful execution
  • Common error scenarios
    This would help users verify their setup is working correctly.

Would you like me to help draft the example outputs section?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2cb34a8 and e4d1223.

📒 Files selected for processing (1)
  • docs/quick-start.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/quick-start.md

[uncategorized] ~37-~37: Possible missing comma found.
Context: ... the TaskContext, Serializable, or Data types as we'll > explain them in later sectio...

(AI_HYDRA_LEO_MISSING_COMMA)


[uncategorized] ~188-~188: Possible missing comma found.
Context: ...ent can be compiled like any normal C++ application except that we need to link it to the S...

(AI_HYDRA_LEO_MISSING_COMMA)

🪛 Markdownlint (0.37.0)
docs/quick-start.md

46-46: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


62-62: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)


121-121: null
Emphasis used instead of a heading

(MD036, no-emphasis-as-heading)

🔇 Additional comments (2)
docs/quick-start.md (2)

86-87: Add instructions for obtaining the Spider directory

The guide assumes Spider is available in a subdirectory but doesn't explain how to obtain it.


226-230: Add security warning for credentials

The example uses hardcoded credentials in plain text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants